Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve macro error handling #9497

Merged
merged 2 commits into from
Jan 21, 2024
Merged

Improve macro error handling #9497

merged 2 commits into from
Jan 21, 2024

Conversation

devongovett
Copy link
Member

This fixes two issues with macro error handling:

  1. If you made a syntax error in a macro (or any dev dependency), fixing it would not trigger a rebuild in watch mode. This is because errors thrown by transformers short-circuit adding dev dependencies for plugins in Transformation.js. This is fixed by adding a try...finally around that code.
  2. If you have a single macro import that is used multiple times in the same file, it will output the same load error multiple times. Now we distinguish between errors loading the macro file (e.g. resolution + syntax), from execution errors and only show load errors once. The diagnostic also shows the import statement as the location rather than each call.

@mischnic
Copy link
Member

mischnic commented Jan 21, 2024

There's a failing test on Windows (though introduced before this PR):

  4) macros
       should throw a diagnostic when a macro cannot be resolved:

      AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

[
  {
    codeFrames: [
      {
        codeHighlights: [
          {
            end: {
              column: 57,
              line: 1
            },
            message: undefined,
            start: {
              column: 1,
              line: 1
            }
          }
        ],
        filePath: 'D:\\a\\parcel\\parcel\\packages\\core\\integration-tests\\test\\macros\\13\\index.js'
      }
    ],
    hints: null,
    message: 'Error loading macro: Could not resolve module "./macro.js" from "D:\\...

should loosely deep-equal

[
  {
    codeFrames: [
      {
        codeHighlights: [
          {
            end: {
              column: 57,
              line: 1
            },
            message: undefined,
            start: {
              column: 1,
              line: 1
            }
          }
        ],
        filePath: 'D:\\a\\parcel\\parcel\\packages\\core\\integration-tests\\test\\macros\\13\\index.js'
      }
    ],
    hints: null,
    message: 'Error loading macro: Could not resolve module "./macro.js" from "D:\\...
      + expected - actual

               "filePath": "D:\\a\\parcel\\parcel\\packages\\core\\integration-tests\\test\\macros\\13\\index.js"
             }
           ]
           "hints": [null]
      -    "message": "Error loading macro: Could not resolve module \"./macro.js\" from \"D:\\\\a\\\\parcel\\\\parcel\\\\packages\\\\core\\\\integration-tests\\\\test\\\\macros\\\\13\\\\index.js\""
      +    "message": "Error loading macro: Could not resolve module \"./macro.js\" from \"D:\\a\\parcel\\parcel\\packages\\core\\integration-tests\\test\\macros\\13\\index.js\""
           "origin": "@parcel/transformer-js"
         }
       ]
      
      at Context.<anonymous> (test\/macros.js:377:14)

@devongovett
Copy link
Member Author

Ah. Windows CI is so flaky I missed it. 😕

Will address separately.

@devongovett devongovett merged commit b967689 into macro-const Jan 21, 2024
14 of 16 checks passed
@devongovett devongovett mentioned this pull request Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants